fix(cli): align test/ error messages with 4-ingredient strategy#1259
Merged
John-David Dalton (jdalton) merged 2 commits intomainfrom Apr 24, 2026
Merged
fix(cli): align test/ error messages with 4-ingredient strategy#1259John-David Dalton (jdalton) merged 2 commits intomainfrom
John-David Dalton (jdalton) merged 2 commits intomainfrom
Conversation
Rewrites the Socket-JSON contract validator and auth-flow mocks under
packages/cli/src/test/ and packages/cli/test/ to follow the What /
Where / Saw vs. wanted / Fix strategy from CLAUDE.md.
Sources:
- src/test/json-output-validation.mts (6 throws): each violation now
spells out the full Socket-JSON contract, the received value, and
a concrete fix ("add ok:true", "return empty object", etc.).
Long output payloads are truncated to 200 chars in the message so
errors stay readable.
- src/test/mocks/socket-auth.mts (2 throws): "Authentication failed"
and "OAuth timeout" now call out that they come from a test
fixture and point at the configuration flag to change.
- test/json-output-validation.mts (2 non-throwing returns): message
values now include the exit code / parse error and a stdout
preview so failing tests diagnose themselves.
- test/smoke.sh (6 labels): updated to mirror the TS validator so
the bash and JS harnesses produce the same wording.
Tests: full suite (343 files / 5225 tests) still passes. No
assertions touched — the unrelated "Authentication failed" hits
in other tests are test fixtures constructing their own Errors,
not references to the mock.
Follows strategy from #1254. Continues #1255-#1258.
1 task
Switch `(e as Error).message` to `e instanceof Error ? e.message : String(e)` so that when a non-Error value is thrown (strings, objects, null) the error message stays informative instead of becoming 'undefined'. Same fix as applied to #1260 (iocraft.mts) after Cursor bugbot flagged the pattern on that PR.
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a8efd4f. Configure here.
James Tu (jmsjtu)
approved these changes
Apr 24, 2026
John-David Dalton (jdalton)
added a commit
that referenced
this pull request
Apr 24, 2026
…, terminal) (#1260) * fix(cli): align utils/ miscellaneous error messages with 4-ingredient strategy Final PR in the error-message series. Covers everything not already touched by #1255-#1259: utils/basics, utils/config, utils/fs, utils/git, utils/npm, utils/promise, utils/terminal, and the flags module at the root of the CLI tree. Sources: - flags.mts: 2 throws (--max-old-space-size, --max-semi-space-size) — name the flag, show the offending value, suggest a concrete megabyte value. - utils/config.mts: 1 throw (SOCKET_CLI_CONFIG base64 decode) — explains the replacement-character symptom and how to re-encode. - utils/basics/vfs-extract.mts: 4 throws (SEA VFS extraction for Python + security tools) — name the missing paths, the exit codes, and point at the "rebuild the SEA binary" fix. - utils/promise/queue.mts: 1 throw (PromiseQueue concurrency guard) — show the offending value and suggest 4/8. - utils/npm/spec.mts: 1 throw (PURL conversion) — show the input, state what a valid npm spec looks like. - utils/git/operations.mts: 1 throw (git-not-on-PATH) — point at install and the local-path env-var override. - utils/git/gitlab-provider.mts: 2 throws (no token, PR creation after retries) — name the token scope, the retry count, the repo/head refs. - utils/fs/path-resolve.mts: 1 throw (npm path-walk iteration cap) — name the start path, current directory, and what usually causes the cycle (symlinks). - utils/terminal/iocraft.mts: 1 throw (native-module load failure) — show the underlying error and the offending platform/arch triple. Skipped (already informative): - github-provider.mts pass-through errors (forward inner CResult cause/message) - gitlab-provider.mts try/catch wrappers that call formatErrorWithDetail (inner error has context) - 'process.exit called' sentinel throws in npm/pnpm/yarn/with- subcommands paths (test harness re-raise markers, not user-facing) Tests updated: - test/unit/utils/promise/queue.test.mts (2 assertions) - test/unit/utils/npm/spec.test.mts (2 assertions) - test/unit/utils/git/gitlab-provider.test.mts (3 assertions) Full suite (343 files / 5225 tests) passes. Completes the series: #1255 (commands/) → #1256 (utils/dlx/) → #1257 (utils/update + utils/command/) → #1258 (env/ + constants/) → #1259 (test/) → this. * fix(cli): address Cursor bugbot findings on error messages Four issues flagged by Cursor bugbot on #1260: 1. (Medium) gitlab-provider.mts: error said 'check GL_TOKEN permissions' but the actual env var is GITLAB_TOKEN (as the same file's getGitLabToken confirms). Fixed to GITLAB_TOKEN. 2. (Medium) git/operations.mts: error suggested 'set SOCKET_CLI_GIT_PATH to point at a specific binary' — that env var is not read anywhere. Removed the false suggestion; kept the real fix (install git and put it on PATH) with package-manager examples. 3. (Low) terminal/iocraft.mts: '(e as Error).message' evaluates to undefined when a non-Error is thrown. Switched to 'e instanceof Error ? e.message : String(e)' for safe stringification. 4. (Low) gitlab-provider.mts: error said 'after ${retries} retries' but the loop runs attempt 1..retries inclusive — retries is the total attempt count, not retries beyond the first. Reworded to 'attempts'. Matching test assertions updated. Caught by #1260 bugbot review. * chore(cli): use joinAnd + getErrorCause helpers in utils/ misc - basics/vfs-extract.mts: missingTools list now renders as prose via joinAnd('a, b, and c'). - terminal/iocraft.mts: inline `e instanceof Error ? e.message : String(e)` swapped for getErrorCause(e). require() of a native binding can throw non-Error values, so the safe-stringify with UNKNOWN_ERROR fallback is correct here. No behavior change for Error throws.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 6 of the error-message series. Covers test utilities:
packages/cli/src/test/(the Socket-JSON contract validator + auth-flow mocks, which are shared across the test suite) andpackages/cli/test/(the bash smoke harness + its JS counterpart).~16 messages across 4 files. Full test suite (343 files / 5225 tests) still passes.
What's fixed
src/test/json-output-validation.mts(6 throws)Socket CLI guarantees a JSON output contract:
{ok: true, data: ...}on success or{ok: false, message: ...}on failure. This file's validator is the guardrail. Before, it said things like "JSON output missing required 'ok' boolean field" which didn't tell a failing test author what the full contract looked like or how to satisfy it.Before:
JSON output 'ok' should be true when exit code is 0: {full payload...}After:
Socket JSON contract violation: exit code is 0 but "ok" is false (expected true); got: {preview}... — set ok:true when the command succeeds, or return a non-zero exit codeLong payloads are now truncated to 200 chars in the message so stacktraces stay readable.
src/test/mocks/socket-auth.mts(2 throws)throw new Error('Authentication failed')andthrow new Error('OAuth timeout')— before, a test failure here looked like a real auth outage. Now both errors explicitly identify themselves as test fixtures and point at the config flag that controls them.test/json-output-validation.mts(2 non-throwing returns)This is a lighter shim that returns
{ok: false, message: ...}instead of throwing. Updated to match the throw-path messages so a failing integration test shows the parse error and a stdout preview.test/smoke.sh(6 label strings)The bash smoke harness duplicates the JS validator's logic in shell. Updated its labels to mirror the new wording so the two harnesses produce consistent errors.
Tests
Nothing else needed updating —
grepconfirmed the "Authentication failed" and "Unknown error" hits elsewhere are unrelated (tests constructing their ownnew AuthError('Authentication failed')as test data, or matching on a distinct production error insrc/utils/socket/api.mts).Full suite: 5225/5225 tests pass.
Test plan
--jsoncommand and verify the new validator message points at the contract violation clearlyNote
Low Risk
Low risk: changes are confined to test utilities and smoke harness messaging, with no production logic impact aside from clearer failures when tests detect malformed JSON or misconfigured mocks.
Overview
Tightens and standardizes test-time validation of the Socket CLI
--jsonoutput contract by upgrading error messages to explicitly call out contract violations, include actionable guidance, and show a truncated stdout preview for readability.Updates the auth-flow test mocks to throw fixture-specific failures (instead of generic auth errors), and aligns both the JS integration validator and
test/smoke.shlabels with the same clearer parse/contract diagnostics.Reviewed by Cursor Bugbot for commit a8efd4f. Configure here.